feat(core): Custom session timeout and refresh configuration #8342

despairblue · 2024-01-16T09:55:36Z

Summary

This allows configuring the session timeout and refresh via environment variables.

You can test it by starting n8n like this:

env N8N_USER_MANAGEMENT_JWT_DURATION_HOURS=0.005 N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=-1 pnpm dev

0.005 hours is 18 seconds and -1 means that the auto refresh of the cookie/JWT is disabled.
So you can log in, navigate around. Wait 18 seconds. Refresh and see that you're logged out.

If you want to test the auto refresh you can do this:

env N8N_USER_MANAGEMENT_JWT_DURATION_HOURS=0.005 N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS=0.0025 pnpm dev

This will make the session last for 18 seconds and when less than 9 seconds are remaining it will issue a new cookie/JWT.

Reviewing this commit by commit can be easier. I mostly added tests before making changes, to make sure I don't break old behaviour.

Related tickets and issues

https://linear.app/n8n/issue/PAY-1182/custom-session-timeout-configuration
https://linear.app/n8n/issue/DOC-734/add-custom-session-timeout-setting-to-environment-variables-page
n8n-io/n8n-docs#1849

Review / Merge checklist

PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
Docs updated or follow-up ticket created.
Tests included.

A bug is not considered fixed, unless a test is added to prevent it from happening again.
A feature is not complete without tests.

CLAassistant · 2024-01-16T09:55:43Z

All committers have signed the CLA.

Rename `JwtToken.expiresIn` to `JwtToken.expiresInSeconds` to make it clear what unit it's supposed to be. Also it's now seconds and not milliseconds. Reason for that is that seconds are a base unit of the SI, which makes it a good default. Also it's what the JWTs expect in `expireIn` and what the cookie Max-Age property expects. Until now the function returned milliseconds and that was passed into the Max-Age property in `auth/jwt.issueCookie`. This caused no real issue, but it meant that the JWT would always expire before the cookie.

no need to have it in the beforeEach

Additionally I had to wrangle the types a bit. I want to call `await refreshExpiringCookie(...)` in the unit tests, but to do this without TS complaining the type needs account for the function being async. Right now adding the type to the variable binding hides that fact. My workaround for that is to let TS infer the type of `refreshExpiringCookie`. That lead to TS not being able to infer the types of `res` and `next`. To fix that I used `satisfied`. Happy to apply other solutions to this like extending the RequestHandler type and creating an `AsyncRequestHandler` or adding an `Async` type helper and using that. ```ts type Async<T extends (...args: unknown[]) => unknown> = ( ...args: Parameters<T> ) => Promise<ReturnType<T>>; ```

…r than jwtSessionDurationHours

1. use Time constant for more readable time conversion 2. use jest-mock-extended to create users and requests 3. use type guards and jest.fail to remove unecessary assertions

I'll add this as a comment in the review, where we can agree on where the warning should be emitted.

hours vs days

despairblue · 2024-01-19T13:43:09Z

packages/cli/src/config/index.ts

+	// Can't use the logger here, because it depends on the config.
+	// I could validate it in `Start.init()`, but that feels like the wrong seperation of concerns
+	// I could also do it in `refreshExpiringCookie`, but that would then print the message on every request.
+	console.warn(


I can't use the `Container.get(Logger) here, because it depends on the config, which creates a dependency cycle.

Alternative:

I could validate it in Start.init(), but that feels like the wrong separation of concerns

I could also do it in refreshExpiringCookie, but that would then print the message on every request.

despairblue · 2024-01-19T13:46:15Z

packages/cli/src/constants.ts

+ * Please amend conversions as necessary.
+ * Eventually this will superseed `TIME` above
+ */
+export const Time = {


I talked with @ivov about this and he likes it.
I would make a refactor PR removing TIME and replacing it with this.

TIME only allowed converting everything into milliseconds, but I was working with JWT and cookies which all expect seconds, so I thought this here is a bit more versatile.

We can also add a library for this, but the problem seems a bit too trivial to justify adding third party code to make the conversions more readable.

Feel free to disagree and comment.

despairblue · 2024-01-19T13:46:44Z

packages/cli/src/middlewares/auth.ts

+	if (jwtRefreshTimeoutHours === 0) {
+		const jwtSessionDurationHours = config.get('userManagement.jwtSessionDurationHours');
+
+		jwtRefreshTimeoutMilliSeconds = Math.floor(jwtSessionDurationHours * 0.25 * 60 * 60 * 1000);
+	} else {
+		jwtRefreshTimeoutMilliSeconds = Math.floor(jwtRefreshTimeoutHours * 60 * 60 * 1000);
+	}


Happy to use a ternary here instead. Just let me know.

despairblue · 2024-01-19T13:50:05Z

packages/cli/src/middlewares/auth.ts

 			// if cookie expires in < 3 days, renew it.
 			await issueCookie(res, req.user);
 		}
 	}
 	next();
-};
+}) satisfies RequestHandler;


I had to wrangle the types a bit. I want to call
await refreshExpiringCookie(...) in the unit tests, but to do this
without TS complaining about awaiting a non-promise the type needs show that this function is async.

Right now adding the type to the variable binding hides that fact. RequestHandler is synchronous.

My workaround for that is to let TS infer the type of refreshExpiringCookie. That lead to TS not being able to infer the types of res and next anymore. So to fix that I used satisfied, which allows TS to infer the types of the arguments and also infer the type of the function as a whole without the need to create a new type for async request handlers.

Happy to apply other solutions to this like extending the RequestHandler
type and creating an AsyncRequestHandler or adding an Async type
helper and using that.

type Async<T extends (...args: unknown[]) => unknown> = ( ...args: Parameters<T> ) => Promise<ReturnType<T>>;

Let me know what you think.

netroy · 2024-01-19T14:46:01Z

packages/cli/src/Interfaces.ts

@@ -667,7 +667,7 @@ export interface ILicensePostResponse extends ILicenseReadResponse {

 export interface JwtToken {
 	token: string;
-	expiresIn: number;
+	expiresInSeconds: number;


in the context of auth tokens, I've never seen an expiresIn that's not in seconds.
While I do see the additional clarity of mentioning seconds here, I don't think we need to rename the property itself.
I've seen an uptick in the number of long descriptive variable names lately , and I feel like this makes code increasingly harder to read.
IMO variable names should try to be as simple as possible, and all additional relevant metadata should move to JSDocs.

In this particular case, I'd prefer if renamed this back, and add a more descriptive JSDoc instead. That was anyone inspecting the code will still know that this is in seconds.

Happy to change it back, but some background before I do that.

I've never seen an expiresIn that's not in seconds.

I actually thought exactly the same thing. So I assumed this is in seconds:

n8n/packages/cli/src/auth/jwt.ts

Lines 39 to 41 in b6d7757

const signedToken = Container.get(JwtService).sign(payload, {

expiresIn: expiresIn / 1000 /* in seconds */,

});

And it is.

I also thought maxAge is in seconds:

n8n/packages/cli/src/auth/jwt.ts

Lines 88 to 92 in b6d7757

res.cookie(AUTH_COOKIE_NAME, userData.token, {

maxAge: userData.expiresIn,

httpOnly: true,

sameSite: 'lax',

});

Because that's what it is in the browser: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#max-agenumber

And on master we currently pass milliseconds. So I thought I found a bug and fixed it in this PR.

Now I tried it out locally and it turns out express expects milliseconds and converts it internally:
https://expressjs.com/en/api.html#res.cookie

I'm happy to change it back, but I hope that shined some light on why I thought it may be helpful. If express would call their option maxAgeMilliseconds I would have not introduced a bug in this PR 😄

I'll fix the bug first that I have in the PR. Let me know what you think and if you still want me to revert the name change then I will.

I was about to report that there was a bug in the duration, glad you've already figured it out.

I get what you are saying and I agree that it's important to be clear about what unit a variable uses. My point was that we should use more JSDoc for that, and be leaner with our variable names. I've seen long and descriptive names get out of hand in old projects, and that makes code much harder to read because people put more details about what a variable does by putting all the details in the name.

We should instead send PRs (whenever possible) to add JSDoc on the packages we use to have the docs in our editors

I agree. The JSDoc works well enough for me, so I changed the interface back and documented it via JSDoc.

Good thinking with amending the DT types for express.
I hope your DT PR goes through 🤞🏾

See: https://expressjs.com/en/api.html#res.cookie

packages/cli/src/middlewares/auth.ts

github-actions · 2024-01-19T17:17:47Z

✅ All Cypress E2E specs passed

cypress · 2024-01-19T17:17:48Z

3 flaky tests on run #3854 ↗︎

0	338	5	0	3

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 despairblue 🗃️ e2e/*
Project: n8n	Commit: `01e15359f9`
Status: Passed	Duration: 03:33 💡
Started: Jan 22, 2024 8:43 AM	Ended: Jan 22, 2024 8:47 AM

5-ndv.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
NDV > should not retrieve remote options when required params throw errors		`Test Replay` `Screenshots` `Video`

17-sharing.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Sharing > should work for admin role on credentials created by others (also can share it with themselves)		`Test Replay` `Screenshots` `Video`

24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items		`Test Replay` `Screenshots` `Video`

Review all test suite changes for PR #8342 ↗︎

github-actions · 2024-01-22T08:47:07Z

✅ All Cypress E2E specs passed

janober · 2024-01-24T13:00:27Z

Got released with [email protected]

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jan 16, 2024

despairblue added 17 commits January 19, 2024 15:07

add test for cli/auth/jwt

638a6be

add units to the doc string

a9a6749

move mockInstance call to module root

19f43b2

no need to have it in the beforeEach

adding unit test for middleware/auth.ts

fd55dfe

remove TODO commment

567766b

use Time constant to convert between units of time

2509261

add jwtRefreshTimeoutHours and test that it resets to 0 if it's large…

45c90c6

…r than jwtSessionDurationHours

clean up test

7543c9a

1. use Time constant for more readable time conversion 2. use jest-mock-extended to create users and requests 3. use type guards and jest.fail to remove unecessary assertions

make the refresh timeout configurable

4e9bd04

remove comment

928a3ac

I'll add this as a comment in the review, where we can agree on where the warning should be emitted.

remove TODO comment

bf3df40

don't hard code default

a76c071

don't use 2 different units in the test title and in the test's code

fa4dee9

hours vs days

don't recalculate the same value multiple times

1762e68

remove todo comment

24472c1

despairblue force-pushed the pay-1182-custom-session-timeout-configuration branch from f68fea3 to 24472c1 Compare January 19, 2024 14:08

despairblue marked this pull request as ready for review January 19, 2024 14:08

despairblue commented Jan 19, 2024

View reviewed changes

despairblue changed the title ~~feat(cli): custom session timeout configuration~~ feat(core): custom session timeout configuration Jan 19, 2024

despairblue changed the title ~~feat(core): custom session timeout configuration~~ feat(core): custom session timeout and refresh configuration Jan 19, 2024

despairblue changed the title ~~feat(core): custom session timeout and refresh configuration~~ feat(core): Custom session timeout and refresh configuration Jan 19, 2024

netroy reviewed Jan 19, 2024

View reviewed changes

fix: maxAge needs to be milliseconds

27ad95e

See: https://expressjs.com/en/api.html#res.cookie

krynble reviewed Jan 19, 2024

View reviewed changes

packages/cli/src/middlewares/auth.ts Outdated Show resolved Hide resolved

remove outdated comment

d9b6140

despairblue requested review from krynble and netroy January 19, 2024 17:05

krynble previously approved these changes Jan 19, 2024

View reviewed changes

use JSDoc to document the unit instead of suffixing the variable name

29c4496

despairblue dismissed krynble’s stale review via 29c4496 January 19, 2024 21:42

despairblue requested a review from krynble January 19, 2024 21:46

add missing documentation

01e1535

despairblue mentioned this pull request Jan 19, 2024

add documentation for new environment variables that allow configuring the session duration and refresh threshold n8n-io/n8n-docs#1849

Merged

krynble approved these changes Jan 22, 2024

View reviewed changes

despairblue merged commit 07e6705 into master Jan 22, 2024
29 checks passed

despairblue deleted the pay-1182-custom-session-timeout-configuration branch January 22, 2024 08:54

github-actions bot mentioned this pull request Jan 24, 2024

🚀 Release 1.26.0 #8427

Merged

janober added the Released label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Custom session timeout and refresh configuration #8342

feat(core): Custom session timeout and refresh configuration #8342

despairblue commented Jan 16, 2024 •

edited

Loading

CLAassistant commented Jan 16, 2024 •

edited

Loading

despairblue Jan 19, 2024

despairblue Jan 19, 2024

despairblue Jan 19, 2024

despairblue Jan 19, 2024

netroy Jan 19, 2024 •

edited

Loading

despairblue Jan 19, 2024 •

edited

Loading

krynble Jan 19, 2024

netroy Jan 19, 2024

despairblue Jan 19, 2024

github-actions bot commented Jan 19, 2024

cypress bot commented Jan 19, 2024 •

edited

Loading

github-actions bot commented Jan 22, 2024

janober commented Jan 24, 2024

	const signedToken = Container.get(JwtService).sign(payload, {
	expiresIn: expiresIn / 1000 /* in seconds */,
	});

	res.cookie(AUTH_COOKIE_NAME, userData.token, {
	maxAge: userData.expiresIn,
	httpOnly: true,
	sameSite: 'lax',
	});

feat(core): Custom session timeout and refresh configuration #8342

feat(core): Custom session timeout and refresh configuration #8342

Conversation

despairblue commented Jan 16, 2024 • edited Loading

Summary

Related tickets and issues

Review / Merge checklist

CLAassistant commented Jan 16, 2024 • edited Loading

despairblue Jan 19, 2024

Choose a reason for hiding this comment

despairblue Jan 19, 2024

Choose a reason for hiding this comment

despairblue Jan 19, 2024

Choose a reason for hiding this comment

despairblue Jan 19, 2024

Choose a reason for hiding this comment

netroy Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

despairblue Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

krynble Jan 19, 2024

Choose a reason for hiding this comment

netroy Jan 19, 2024

Choose a reason for hiding this comment

despairblue Jan 19, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2024

cypress bot commented Jan 19, 2024 • edited Loading

3 flaky tests on run #3854 ↗︎

Review all test suite changes for PR #8342 ↗︎

github-actions bot commented Jan 22, 2024

janober commented Jan 24, 2024

despairblue commented Jan 16, 2024 •

edited

Loading

CLAassistant commented Jan 16, 2024 •

edited

Loading

netroy Jan 19, 2024 •

edited

Loading

despairblue Jan 19, 2024 •

edited

Loading

cypress bot commented Jan 19, 2024 •

edited

Loading